Repository navigation
[Python] Preserve argument binding for defaulted DoFn elements - #40798
Conversation
Ignore the ordinary default on the implicitly supplied first input when all positional parameters have defaults. This keeps injected parameters and side inputs in their intended positions for process and process_batch. Cover implicit and explicit element parameters, scalar and deferred side inputs, and the existing FnApiRunner batch execution configurations. Fixes apache#19631 Assisted-by: OpenAI Codex
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR fixes issue #19631 where DoFn.process and DoFn.process_batch methods with a default value on the implicit first (element/batch) parameter misbound subsequent special DoFn parameters and side inputs. By stripping the leading ordinary default in _get_arg_placeholders when all positional parameters have defaults, the element slot and remaining parameter placeholders align properly.
🔍 General Feedback
- The fix in
sdks/python/apache_beam/runners/common.pyis minimal, well-scoped, and preserves existing handling for explicitDoFn.ElementParamand special parameter markers (_DoFnParamandBundleFinalizerParam). - Test coverage in
common_test.pyandfn_runner_test.pythoroughly exercises default, positional, keyword, and side-input bindings for bothprocessandprocess_batch.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @damccorm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40798 +/- ##
===========================================
+ Coverage 0 79.90% +79.90%
===========================================
Files 0 493 +493
Lines 0 83482 +83482
===========================================
+ Hits 0 66710 +66710
- Misses 0 16772 +16772
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
waiting on author |
Skip the default-argument loop when no defaults remain, avoiding the negative-zero slice noted in review. Assisted-by: OpenAI Codex
github-actions
Bot
added
size/m
|
Letting workflows run before merging |
Fixes #19631.
When every positional parameter has a default,
_get_arg_placeholderscounts the implicit element as a side-input slot. Forprocess(element=1, timestamp=DoFn.TimestampParam, side_input=3), passing side argument2produces(element, 2, timestamp)instead of(element, timestamp, 2).Skip that first ordinary default in the implicit-element branch, so the existing argument-count and placeholder logic starts at the next parameter. Explicit
ElementParamand leading special-marker handling are unchanged. The same helper fixesprocess_batch; the tests cover that path across the existing FnApiRunner configurations. A Python bugfix note is included inCHANGES.md.Validation on Windows/Python 3.12 with the native Cython module rebuilt using MSVC:
fn_runner_test.py: 465 passed, 81 skipped.BundleBasedDirectRunner: 283 passed, 3 skipped, 5 failed. Rebuilding the exact originalcommon.pyreproduces all five failures in unchanged PTransform tests. The candidate was then restored, rebuilt and its 15 new cases rerun successfully..pyiUTF-8 decoding errors on both versions. No unrelated lint/type-check changes were made.The public issue was also reproduced independently on both BundleBasedDirectRunner and FnApiRunner before the change; both now return the expected values. Cloud runners and other Python versions were not run locally. AI assistance: OpenAI Codex.